Skip to content

Dev#10

Closed
Wil-Collier wants to merge 50 commits into
mainfrom
dev
Closed

Dev#10
Wil-Collier wants to merge 50 commits into
mainfrom
dev

Conversation

@Wil-Collier

@Wil-Collier Wil-Collier commented May 17, 2026

Copy link
Copy Markdown
Collaborator

This pull request introduces several major improvements to the project, including a comprehensive test suite overhaul, a significant refactor of custom-field staging, expanded CI coverage, and enhanced documentation. It also adds a new advisory mutation testing workflow, raises the coverage gate, and documents all recent changes in a detailed changelog. Additionally, it introduces a new language server configuration for multiple languages.

Testing and CI enhancements:

  • Overhauled the test suite by removing duplicated per-resource test files and replacing them with a single parametrized smoke test, added new resources to tests, improved assertion accuracy, and closed coverage gaps. The coverage gate is now set to 95%, and filterwarnings = error is enforced in pytest.ini.
  • Added a comprehensive CI workflow (.github/workflows/ci.yml) that runs linting, type-checking, unit, and integration tests across multiple Python and Pydantic versions, with coverage reporting and concurrency controls.
  • Introduced an advisory mutation testing workflow (.github/workflows/mutation.yml) that runs mutmut on PRs, uploads results as an artifact, and never blocks merges.

Custom-field staging and behavior changes:

  • Refactored custom-field staging for Asset objects: added pending_custom_fields(), improved set_custom_field() and get_custom_field() behaviors, and decoupled staging from Pydantic internals. Now, setting a field to its current value cancels the pending stage, and server responses are correctly folded into the local model.

Documentation and configuration:

  • Added a detailed CHANGELOG.md documenting all major changes, bug fixes, and new features up to version 0.4.0, including internal refactors, test improvements, and documentation updates.
  • Added .kiro/settings/lsp.json with language server configurations for TypeScript, Rust, Python, Java, Go, Ruby, and C++, improving development environment support.

Summary by CodeRabbit

Release Notes

  • New Features

    • Custom field staging and commit workflow for assets
    • Asset file upload/download with progress callbacks
    • Asset restore functionality
    • Structured logging with sensitive header redaction
    • Improved exception hierarchy exported from main package
    • Context manager support for client lifecycle
  • Bug Fixes

    • Better dirty-field tracking for object persistence
    • Enhanced error handling with specific exception types
  • Documentation

    • Updated README with custom field usage guidance
    • Added CHANGELOG documenting all releases
    • Added RELEASING guide for maintainers
  • Refactor

    • Migrated HTTP library from requests to httpx
    • Enhanced retry logic with exponential backoff and Retry-After support

Review Change Stack

- what: replace requests transport with httpx, retry/logging helpers, and eager resource managers
- what: move API objects onto Pydantic v2 with safe dirty tracking for extra fields
- why: prepare the client for clearer HTTP behavior, typed public surface checks, and safer model updates
- risk: breaking change for requests-specific session internals
- what: add the 0.2 changelog and refresh README examples/features
- why: explain the httpx, Pydantic, retry, logging, and compatibility changes
- risk: documentation only
- what: add GitHub Actions for lint, type-check, unit tests, and coverage
- what: update Makefile unit and coverage targets to include contract tests
- why: keep the migration covered across Python 3.11 through 3.13
- risk: local make test still requires PY to point at an environment with pytest
… prevents collection conflict with same-named unit test files
…, stream errors, ValidationError parse failure (tasks 9-12, 15)
…s, retry PATCH/DELETE/respect_retry_after (tasks 16, 17)
…trip, restore lifecycle

- Custom fields: Field -> Fieldset -> Model -> Asset, exercising both top-level
  column-name PATCH and in-place custom_fields dict mutation (README pattern).
- File round-trip: 64KiB random payload, byte-compares upload vs download,
  verifies progress callback, confirms file removal via /delete suffix.
- Restore lifecycle: create -> soft-delete (with marker assertion) -> restore
  -> confirm reachable with no deleted markers.

Closes the unit-vs-real gap on the three highest-risk asset code paths.
When docker/api_key.txt is missing or a directory, the seeder service's
bind-mount './api_key.txt:/api_key.txt' makes Docker auto-create the host
path as a directory, breaking both the seeder's '> /api_key.txt' redirect
and the integration conftest's read_text() with IsADirectoryError.

Fix:
- docker-up: pre-create api_key.txt as an empty regular file if missing
  or if it is a directory.
- docker-down: replace '> docker/api_key.txt' (which silently fails on a
  directory) with 'rm -rf' + 'touch' so the file always ends up as a
  regular empty file.
- test-integration: hard-fail with a clear message if api_key.txt is
  somehow still a directory after docker-up.
…pe-IT

The integration suite was failing with two distinct flake patterns:

1. Connection reset on first run after 'make docker-up'
   The Makefile waited for docker/api_key.txt to be non-empty, but Snipe-IT's
   app container is still booting Apache/PHP at the moment the seeder writes
   the token. The first POSTs hit a half-open socket and got ECONNRESET.
   Fix: add a real API readiness probe that polls /api/v1/users/me with the
   token until it returns 200 (Makefile test-integration target).

2. HTTP 429 on second/rapid runs
   Snipe-IT defaults to API_THROTTLE_PER_MINUTE=240 (4 req/sec). The full
   integration suite hits the limiter. The library's default retry policy
   (HEAD/GET/OPTIONS only, never POST/PATCH/DELETE) is correct for
   production safety but wrong for tests, where Snipe-IT 429s before
   processing the request body so retries are safe.
   Fix:
   - Bump docker/.env API_THROTTLE_PER_MINUTE to 1200 (20 req/sec).
   - Configure the real_snipeit_client fixture to retry mutating methods
     on 429 with max_retries=5; document why this is test-env-only.

E2E test fixes from first real-Snipe-IT run:

- test_asset_files_e2e: Snipe-IT rejects .bin files (extension allowlist)
  and renames stored files with an asset-{id}-{random}- prefix. Switch to
  .txt with 64KiB of hex chars; match by suffix instead of exact filename.

- test_custom_fields_e2e: setattr(asset, '_snipeit_*', value) is silently
  dropped by ApiObject's underscore-guard, so save() drops the change. Use
  mark_dirty() workaround. Document this in README. The README's nested
  in-place mutation pattern is also silently ignored by Snipe-IT — test
  now skips with diagnostic instead of failing.

Run results: 17 passed, 2 skipped (with clear reasons), 0 failed.
Stable across consecutive runs.
Introduces `get_custom_field` and `set_custom_field` helper methods on
the
`Asset` model. These methods abstract away the complexities of
Snipe-IT's
custom field API, which uses different shapes for reading and writing.

The `get_custom_field` method allows retrieving custom field values by
their
display label, providing a default value if the field is not found.

The `set_custom_field` method stages changes to custom fields by their
display label. When `save()` is called, these staged changes are
translated
into the correct top-level keys in the PATCH request, using the
underlying
column names as expected by the Snipe-IT API. This avoids the common
pitfall
of directly setting attributes starting with `_`, which are ignored by
the
library's dirty tracker.
- Add Asset.pending_custom_fields() for inspecting staged state
- Fix set_custom_field() + save() not requiring refresh() between calls
- Fold PATCH response column-name keys back into nested shape
- get_custom_field() now returns server value, not staged value
- Setting a field back to server value cancels the pending stage
- Staging moved to dedicated _pending_custom_fields PrivateAttr
- Remove NOTICE from license-files (file doesn't exist)
- Migrate [mutmut] config from setup.cfg to pyproject.toml
- Delete setup.cfg
- Fix .gitignore: remove *.lock, add .kiro/
- Track uv.lock for reproducible builds
@coderabbitai

coderabbitai Bot commented May 17, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0df2689b-c3e6-4ff8-91b3-4920572dfae3

📥 Commits

Reviewing files that changed from the base of the PR and between 11b0859 and 7227ed3.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (92)
  • .github/workflows/ci.yml
  • .github/workflows/mutation.yml
  • .gitignore
  • .kiro/settings/lsp.json
  • CHANGELOG.md
  • LICENSE
  • Makefile
  • README.md
  • RELEASING.md
  • docker/.env
  • docker/README.md
  • docker/docker-compose.yml
  • docs/audit.json
  • docs/groups.json
  • docs/maintenances.json
  • docs/reports.json
  • docs/settings.json
  • docs/split_api.py
  • pyproject.toml
  • pytest.ini
  • setup.cfg
  • snipeit/__init__.py
  • snipeit/_log.py
  • snipeit/_retry.py
  • snipeit/client.py
  • snipeit/client.pyi
  • snipeit/exceptions.py
  • snipeit/py.typed
  • snipeit/resources/accessories.py
  • snipeit/resources/assets.py
  • snipeit/resources/assets/__init__.py
  • snipeit/resources/assets/files.py
  • snipeit/resources/assets/labels.py
  • snipeit/resources/assets/manager.py
  • snipeit/resources/assets/model.py
  • snipeit/resources/base.py
  • snipeit/resources/categories.py
  • snipeit/resources/companies.py
  • snipeit/resources/components.py
  • snipeit/resources/consumables.py
  • snipeit/resources/departments.py
  • snipeit/resources/fields.py
  • snipeit/resources/fieldsets.py
  • snipeit/resources/licenses.py
  • snipeit/resources/locations.py
  • snipeit/resources/manufacturers.py
  • snipeit/resources/models.py
  • snipeit/resources/status_labels.py
  • snipeit/resources/suppliers.py
  • snipeit/resources/users.py
  • tests/__init__.py
  • tests/conftest.py
  • tests/contract/test_public_surface.py
  • tests/integration/conftest.py
  • tests/integration/resources/test_asset_files_e2e.py
  • tests/integration/resources/test_asset_restore_e2e.py
  • tests/integration/resources/test_assets.py
  • tests/integration/resources/test_companies.py
  • tests/integration/resources/test_custom_fields_e2e.py
  • tests/integration/resources/test_suppliers.py
  • tests/unit/resources/test_accessories.py
  • tests/unit/resources/test_assets.py
  • tests/unit/resources/test_assets_extra.py
  • tests/unit/resources/test_assets_labels.py
  • tests/unit/resources/test_base.py
  • tests/unit/resources/test_categories.py
  • tests/unit/resources/test_components.py
  • tests/unit/resources/test_consumables.py
  • tests/unit/resources/test_departments.py
  • tests/unit/resources/test_fields.py
  • tests/unit/resources/test_fieldsets.py
  • tests/unit/resources/test_licenses.py
  • tests/unit/resources/test_locations.py
  • tests/unit/resources/test_manufacturers.py
  • tests/unit/resources/test_models.py
  • tests/unit/resources/test_pagination.py
  • tests/unit/resources/test_resources_smoke.py
  • tests/unit/resources/test_resources_specific.py
  • tests/unit/resources/test_shape_validation.py
  • tests/unit/resources/test_status_labels.py
  • tests/unit/resources/test_users.py
  • tests/unit/test_assets_endpoints.py
  • tests/unit/test_client.py
  • tests/unit/test_client_edge_cases.py
  • tests/unit/test_client_properties.py
  • tests/unit/test_exceptions.py
  • tests/unit/test_logging.py
  • tests/unit/test_property_apiobject.py
  • tests/unit/test_repr.py
  • tests/unit/test_repr_exact.py
  • tests/unit/test_retries.py
  • tests/unit/test_streaming_download.py

📝 Walkthrough

Walkthrough

Switches to an httpx-based client with custom retry transport and structured logging, refactors ApiObject/Managers and splits assets into model/manager/mixins, migrates tests to httpx_mock with expanded coverage, adds CI and mutation workflows, Dockerized integration setup, updates packaging/configs, and adds docs/license/changelog.

Changes

Core Client and Resources Refactor

Layer / File(s) Summary
HTTPX client, retry, and logging
snipeit/client.py, snipeit/_retry.py, snipeit/_log.py, snipeit/exceptions.py
Replaces requests with httpx, adds RetryTransport/backoff and structured logging, tightens status/error handling, streaming helpers, and docstrings.
ApiObject and managers rewrite
snipeit/resources/base.py
Moves to Pydantic v2 with snapshot-based dirty tracking, payload normalization, and adjusted Manager behaviors.
Assets split and extensions
snipeit/resources/assets/*, snipeit/resources/assets.py (removed)
Splits assets into model/manager plus files/labels mixins; enhances custom-field staging, file and label handling; removes monolithic module.
Other resources path normalization
snipeit/resources/*
Renames _path to _resource_path and updates __repr__s and manager wiring across resources.

Testing (Unit and Integration)

Layer / File(s) Summary
Unit tests migration/expansion
tests/unit/**/*
Migrates to httpx_mock, adds extensive coverage for assets custom-fields, retries, streaming, logging, shape validation, smoke/specific resource tests, and client edge cases. Removes duplicated per-resource files replaced by parametrized suites.
Integration tests and fixtures
tests/integration/**/*
Adds e2e tests for assets files, restore lifecycle, companies, suppliers; refactors env handling and client fixtures (retry/no-retry).
Contract tests
tests/contract/test_public_surface.py
Asserts stable public API surface, exception hierarchy, client constructor signature, managers, and key methods.

CI, Tooling, and Local Stack

Layer / File(s) Summary
GitHub Actions CI and mutation
.github/workflows/*.yml
Adds matrix CI (lint/type/test/coverage) and advisory mutation workflow with artifacts.
Make/Docker integration flow
Makefile, docker/*
Adds readiness checks (token and /users/me), manages api_key file lifecycle, raises local API rate limit, and updates seeder steps.
Project config updates
pyproject.toml, pytest.ini, setup.cfg, .gitignore, .kiro/settings/lsp.json
Updates metadata/deps (httpx/pydantic), dev groups, mutmut config, warnings-as-errors, ignores, and LSP profiles.

Docs and Repository Metadata

Layer / File(s) Summary
Docs and metadata
README.md, CHANGELOG.md, RELEASING.md, LICENSE, snipeit/__init__.py
Rewrites README with pitfalls/custom-fields, adds CHANGELOG, releasing guide, Apache-2.0 license, and re-exports exceptions. Removes unused OpenAPI artifacts and splitter script.

Sequence Diagram(s)

sequenceDiagram
  participant Dev
  participant CI(gha) as CI (GitHub Actions)
  participant Docker
  participant SnipeIT as Local Snipe-IT
  Dev->>CI(gha): push/PR triggers
  CI(gha)->>Docker: docker compose up (seed)
  Docker-->>SnipeIT: start app + write api_key.txt
  CI(gha)->>SnipeIT: poll /api/v1/users/me (bearer token)
  SnipeIT-->>CI(gha): 200 OK
  CI(gha)->>CI(gha): run unit/contract/integration tests
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120+ minutes

Possibly related PRs

  • lfctech/snipeit-python-api#8 — Earlier implementation of asset restore/audit/files/labels behaviors that this PR reorganizes into new assets modules.

Poem

I hop through headers, mask the keys,
Retries drum softly: backoff, please.
New burrow—httpx—so neat and spry,
Assets split, their labels fly.
CI bells ring, mutations play;
I thump approval—ship today! 🐇✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant